Add type configuration support to the typescript cli plugin #67
Conversation
|
looks like build checks are failing |
|
Thanks for triggering the CI and as you note there is one test failure:
We'll investigate and fix. |
| configuration_models = resolve_models( | ||
| project.configuration_schema, "TypeConfigurationModel" | ||
| ) |
There was a problem hiding this comment.
would be great to achieve parity with other plugins:
configuration_schema_path =project.root / project.configuration_schema_filename
project.write_configuration_schema(configuration_schema_path)
There was a problem hiding this comment.
@ammokhov Not sure what this does. As far as I know, we want all the models, TypeConfiguration included to be generated within models.ts. Am I missunderstanding something?
|
FYI, the python test failing is because the test tries to build a sample TypeScript project, which uses the currently released library. But obviously, this doesn't have the new code we added. The solution would be to update the python test
|
|
To summarize - we believe the CI test failures are due to a bug in the project's test framework, not this commit, where the test setup creates and tests a TS project which is wired to use the released version of this plugin, rather than the development version of the plugin. Presumably for some code changes this is okay, but if the code change is such that the TS project won't run without the updated plugin, these tests will fail. We can reproduce locally, and as @tbouron says, if we manually change the |
this makes (1) new version generated code backwards compatible with old version runtime framework (it just won't get type configuration info), and (2) new version runtime framework backwards compatible with old version generated code (it will ignore type configuration info) resulting signature is not especially clean but backwards compatibility is worth it.
|
We've found a sneaky way which seems to allow for backwards compatibility: the (1) new version generated code is backwards compatible with old version runtime framework (it just won't get type configuration info), and This should allow the tests to pass, degrade gracefully if not using the latest version of things. Users should expect TS transpile-time errors about signatures of their handler methods, which now take an additional cc @ammokhov - Could you kick off the CI workflow to see if it passes here? (We have now confirmed that it passes locally.) |
|
The last couple commits tidy up some unrelated build failures (a test re JS error messages, and a new linter rule in Python) so that -- hopefully -- the build can finally pass reliably! |
Codecov Report
@@ Coverage Diff @@
## master #67 +/- ##
==========================================
- Coverage 98.93% 98.38% -0.55%
==========================================
Files 13 13
Lines 1215 1237 +22
Branches 212 217 +5
==========================================
+ Hits 1202 1217 +15
- Misses 13 19 +6
- Partials 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| * @param logger Logger to proxy requests to default publishers | ||
| */ | ||
| @handlerEvent(Action.Delete) | ||
| public async delete( | ||
| session: Optional<SessionProxy>, | ||
| request: ResourceHandlerRequest<ResourceModel>, | ||
| callbackContext: CallbackContext, | ||
| logger: LoggerProxy | ||
| logger: LoggerProxy, | ||
| typeConfiguration: TypeConfigurationModel, |
There was a problem hiding this comment.
I see what you mean about changing the order. You guys are the experts on TypeScript here, but curious as to why we cannot use syntax like typeConfiguration?: TypeConfigurationModel here
There was a problem hiding this comment.
i also wondered why ? or Optional<> wasn't used here -- technically I think it should be present on callback and logger as well as typeConfiguration. possibly the types admin null already or the signature isn't needed. but we tried to match existing patterns as much as possible since this isn't our codebase.
what we had wanted to do and did previously was to include the typeConfiguration before the logger, to match the java signature. but that is what caused the problem.
previous versions of the framework code of course call the old signature, ie delete(session, request, callback, logger).
in the test failure we were seeing, that old framework code is running, making that exact call, but against a handler ^ based on the new template, expecting the new signature, so with the changes we had made earlier it thinks the 4th parameter is a typeConfiguration when the caller gave a logger there. (the new framework called the new signature, so if you ran new versions at both places it worked fine.)
in the new code because we haven't changed any existing fields, just appended a new 5th, it quite happily accepts calls from the old framework to new code or even new framework to old code -- in the former case it calls the signature with 4 arguments, and those args match, and it passes undefined in for the typeConfiguration, which works fine at runtime since the TS type info is thrown away when converted to JS; and in the latter case it includes the new 5th argument as a typeConfiguration but old handlers only take 4 args which all match and it just ignores the 5th.
| @@ -7,8 +7,8 @@ repos: | |||
| - repo: https://github.com/ambv/black | |||
| rev: stable | |||
| hooks: | |||
| - id: black | |||
| # language_version: python3.6 | |||
| - id: black | |||
There was a problem hiding this comment.
was this indent accidental? if so, is it possible to remove this file from the commit?
There was a problem hiding this comment.
probably an IDE did that automatically or a linter wanted it. yaml doesn't care and conventions vary whether list items should be indented two spaces or not. i note all the other blocks in this file do indent list items so this change makes it consistent. i'd recommend keeping but happy to remove if you prefer.
|
Thank you for your contribution! This is great addition to the plugin |
This pull request adds type configuration support to cfn resources generated by the cli using the typescript plugin. This matches the same functionality in the java cli plugin.